-
Notifications
You must be signed in to change notification settings - Fork 294
Support pytest 4 #429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support pytest 4 #429
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could you fix the flake8 errors?
./html5lib/tests/test_sanitizer.py:71:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:72:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:75:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:76:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:79:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:80:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:83:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:84:20: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:97:16: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:98:16: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:105:16: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:106:16: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:114:16: E128 continuation line under-indented for visual indent
./html5lib/tests/test_sanitizer.py:115:16: E128 continuation line under-indented for visual indent
Better? |
Good, thanks! Flake8 now passes on Travis CI. |
Codecov Report
@@ Coverage Diff @@
## master #429 +/- ##
==========================================
- Coverage 90.52% 88.69% -1.83%
==========================================
Files 50 50
Lines 6973 6952 -21
Branches 1328 1308 -20
==========================================
- Hits 6312 6166 -146
- Misses 502 603 +101
- Partials 159 183 +24
Continue to review full report at Codecov.
|
It fails here with pytest 5.2.4, perhaps pytest 5 changed again? I changed the line
to
and it works fine now. Would you consider to include the change too? The error:
|
That change makes sense to me. Can you go ahead with that? |
Not sure, what’s wrong now. Is it some bug inside of pytest? Do I do something wrong? Locally, it passes without any problem both with pytest 4.6.6 and pytest 5.2.4. |
The original non-decorator pytest.mark.skipif line looks wrong to me from the beginning. It should be a decorated @pytest.mark.skipif in the next line IMHO. |
OK, I am too stupid and too sleepy, right now. Could I get a proper diff of what I should do, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majority of these functions that yield tests all yield the same test function, right? Can we not just rename those functions, rename the functions they call, and call the existing functions with @pytest.mark.parametrize
?
i.e., def test_encoding
becomes def generate_encoding
, the first argument from the yields is dropped, and then we end up with:
@pytest.mark.parameterize("data, encoding", generate_encoding())
def test_parser_encoding(data, encoding)
(renamed from runParserEncodingTest
?)
@@ -95,12 +95,13 @@ def runPreScanEncodingTest(data, encoding): | |||
assert encoding == stream.charEncoding[0].name, errorMessage(data, encoding, stream.charEncoding[0].name) | |||
|
|||
|
|||
@pytest.mark.skip(reason="broken under pytest4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be fixed before we can move to 4.x
html5lib/tests/test_treewalkers.py
Outdated
@@ -99,7 +99,7 @@ def test_treewalker_six_mix(): | |||
|
|||
for tree in sorted(treeTypes.items()): | |||
for intext, attrs, expected in sm_tests: | |||
yield runTreewalkerEditTest, intext, expected, attrs, tree | |||
runTreewalkerEditTest(intext, expected, attrs, tree) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, we do want these as separate tests.
See html5lib#429 for upgrading to >= 4
See html5lib#429 for upgrading to >= 4
See #429 for upgrading to >= 4
Fixed by #497 |
Rebase of #414 by @hroncok on the top of the current master, and two small nits fixed.